-
Notifications
You must be signed in to change notification settings - Fork 472
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update tests for String.prototype.matchAll #1587
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@ljharb There's no rush, but I'm unclear what I should do next to get this merged. Are there others I need to add to review? Thanks! |
@peterwmwong altho i have the commit bit, I don't think I should be the one to merge this :-) maybe @rwaldron or @leobalter could take a look? |
As per (tc39/proposal-string-matchall#35), the call to IsRegExp after CreateRegExp was removed and additional checking was replaced by an Assert. Updates to Test262 has been submitted: tc39/test262#1587 Bug: v8:6890 Change-Id: I942b6846bb46cf85b1ea5566f9c19de7d2dbf03e Reviewed-on: https://chromium-review.googlesource.com/1086419 Reviewed-by: Jakob Gruber <jgruber@chromium.org> Reviewed-by: Mathias Bynens <mathias@chromium.org> Commit-Queue: Peter Wong <peter.wm.wong@gmail.com> Cr-Commit-Position: refs/heads/master@{#53539}
I wonder if we should expect a new behavior for the calls in the tests that are now removed here. In any case, I'd like to preserve the tests matching the same or modified behavior matching the current proposal spec. Consider we have implementations already observing these tests, now Test262 should also tell what's new to consider, right? |
@leobalter The spec change is the removal of a call to @ljharb correct me if I'm wrong, but I'm not sure what meaningful test could be written to verify that that particular |
@peterwmwong you could keep the test that .calls with an object literal, but assert that its toString is called? |
@ljharb, are you thinking something like these tests below? // Copyright (C) 2018 Peter Wong. All rights reserved.
// This code is governed by the BSD license found in the LICENSE file.
/*---
esid: pending
description: String coercion of `string` argument
info: |
RegExp.prototype [ @@matchAll ] ( string )
[...]
3. Return ? MatchAllIterator(R, string).
MatchAllIterator ( R, O )
1. Let S be ? ToString(O).
features: [Symbol.matchAll]
includes: [compareArray.js, compareIterator.js, regExpUtils.js]
---*/
var str = 'a*b';
var obj = {
toString() {
return str;
}
};
var regexp = /\w/g;
assert.compareIterator(regexp[Symbol.matchAll](obj), [
matchValidator(['a'], 0, str),
matchValidator(['b'], 2, str)
]); // Copyright (C) 2018 Jordan Harband. All rights reserved.
// This code is governed by the BSD license found in the LICENSE file.
/*---
esid: pending
description: String coercion of string parameter
info: |
RegExp.prototype [ @@matchAll ] ( string )
[...]
3. Return ? MatchAllIterator(R, string).
MatchAllIterator ( R, O )
1. Let S be ? ToString(O).
features: [Symbol.matchAll]
---*/
var obj = {
valueOf() {
$ERROR('This method should not be invoked.');
},
toString() {
throw new Test262Error('toString invoked');
}
};
assert.throws(Test262Error, function() {
/toString value/[Symbol.matchAll](obj);
}); |
Yup, both of those look great :-) |
f47c0d4
to
ad6bcdf
Compare
@ljharb @leobalter Huzzah! I have added a new test that will distinguish between implementations that have the spec update vs not: // Copyright (C) 2018 Peter Wong. All rights reserved.
// This code is governed by the BSD license found in the LICENSE file.
/*---
esid: pending
description: IsRegExp should only be called once
info: |
RegExp.prototype [ @@matchAll ] ( string )
[...]
3. Return ? MatchAllIterator(R, string).
MatchAllIterator ( R, O )
[...]
2. If ? IsRegExp(R) is true, then
[...]
3. Else,
a. Let flags be "g".
b. Let matcher be ? RegExpCreate(R, flags).
features: [Symbol.match, Symbol.matchAll]
---*/
var internalCount = 0;
Object.defineProperty(RegExp.prototype, Symbol.match, {
get: function() {
++internalCount;
return true;
}
});
var count = 0;
var o = {
get [Symbol.match]() {
++count;
return false;
}
};
RegExp.prototype[Symbol.matchAll].call(o, '1');
assert.sameValue(0, internalCount);
assert.sameValue(1, count); I verified the test fails without my latest CL to V8: https://chromium-review.googlesource.com/c/v8/v8/+/1086419 I have preemptively updated my commit with this new test. |
As per spec changes (tc39/proposal-string-matchall#35), removed tests related to the removed IsRegExp call. To prevent older implementations (not observing spec change) from passing, added a new test to verify the reduced number of observable calls to IsRegExp. Also fix a misreference in `features` frontmatter.
ad6bcdf
to
bbad948
Compare
Thanks for the follow ups, @peterwmwong! |
As per spec changes (tc39/proposal-string-matchall#35), removed tests related to the removed IsRegExp call.
Also fix a misreference in
features
frontmatter./cc @ljharb